-
Notifications
You must be signed in to change notification settings - Fork 538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Mono.Android] Java.Lang.Object.GetObject<T>()
implementation
#9630
Conversation
return Java.Lang.Object.GetObject (elem, JniHandleOwnership.TransferLocalRef, type); | ||
return GetObject (elem, type); | ||
|
||
// FIXME: https://github.com/xamarin/xamarin-android/issues/8724 | ||
// Since a Dictionary<Type, Func> is used here, the trimmer will not be able to properly analyze `Type t` | ||
// error IL2111: Method 'lambda expression' with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the method. | ||
[UnconditionalSuppressMessage ("Trimming", "IL2067", Justification = "[DynamicallyAccessedMembers] manually added to all callers")] | ||
static object? GetObject (IntPtr e, Type t) => | ||
Java.Lang.Object.GetObject (e, JniHandleOwnership.TransferLocalRef, t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find a way to fix this properly yet. I think the pattern of:
- Create a
Dictionary<Type, Func>
where one of the parameters needs[DynamicallyAccessedMembers]
- If you decorate the lambda, or create a method...
- The trimmer says: "Method 'lambda expression' with parameters or return value with
DynamicallyAccessedMembersAttribute
is accessed via reflection. Trimmer can't guarantee availability of the requirements of the method."
This situation just isn't going to make the trimmer happy.
For now, I linked to:
The future fix might be to remove the Dictionary
in favor of switch
, that seems like the trimmer could more easily understand.
eff56dd
to
cc52218
Compare
Context: dotnet/android#9630 As #9630 required new declarations of `DynamicallyAccessedMemberTypes.Interfaces`, we intestigated to see *why* these were needed in Java.Interop. There are two cases: var listIface = typeof (IList<>); var listType = (from iface in type.GetInterfaces ().Concat (new[]{type}) where (listIface).IsAssignableFrom (iface.IsGenericType ? iface.GetGenericTypeDefinition () : iface) select iface) .FirstOrDefault (); This first one, if `IList<>` were trimmed away. We don't actually care, as everything in this code path would still work. We can suppress the warning instead. The second case: JniValueMarshalerAttribute? ifaceAttribute = null; foreach (var iface in type.GetInterfaces ()) { marshalerAttr = iface.GetCustomAttribute<JniValueMarshalerAttribute> (); if (marshalerAttr != null) { if (ifaceAttribute != null) throw new NotSupportedException ($"There is more than one interface with custom marshaler for type {type}."); ifaceAttribute = marshalerAttr; } } if (ifaceAttribute != null) return (JniValueMarshaler) Activator.CreateInstance (ifaceAttribute.MarshalerType)!; Feels like we should be able to remove this code completely. With these changes we can remove `DynamicallyAccessedMemberTypes.Interfaces`. I also introduced `build-tools/trim-analyzers/trim-analyzers.props` that will setup the appropriate trimmer MSBuild properties to make trimmer warnings an error. This should keep us from accidentally creating warnings. I only use this setting in projects that were already using `$(EnableAotAnalyzer)`.
) Context: dotnet/android#9630 Context: ec2813a As dotnet/android#9630 required new declarations of `DynamicallyAccessedMemberTypes.Interfaces`, we intestigated to see *why* these were needed in Java.Interop. There are two cases that involve `.Interfaces` within `JniRuntime.JniValueManager.GetValueMarshaler(Type)`: The first case is around attempts to special-case array marshaling for builtin types: var listIface = typeof (IList<>); var listType = (from iface in type.GetInterfaces ().Concat (new[]{type}) where (listIface).IsAssignableFrom (iface.IsGenericType ? iface.GetGenericTypeDefinition () : iface) select iface) .FirstOrDefault (); If `IList<T>` is trimmed away (lol), then *we don't care*, as everything else within `GetValueMarshaler(Type)` still works, and if we can't get a value marshaler for `int[]` (which implements `IList<T>`!), then so be it. Suppress this IL2070 message. The second case is around looking for `[JniValueMarshaler]` on implemented interface types, from ec2813a: JniValueMarshalerAttribute? ifaceAttribute = null; foreach (var iface in type.GetInterfaces ()) { marshalerAttr = iface.GetCustomAttribute<JniValueMarshalerAttribute> (); if (marshalerAttr != null) { if (ifaceAttribute != null) throw new NotSupportedException ($"There is more than one interface with custom marshaler for type {type}."); ifaceAttribute = marshalerAttr; } } if (ifaceAttribute != null) return (JniValueMarshaler) Activator.CreateInstance (ifaceAttribute.MarshalerType)!; It is to support the scenario namespace Android.Runtime { [JniValueMarshaler(typeof(IJavaObjectValueMarshaler))] partial interface /* Android.RuntimIJavaObject {} } namespace Java.Lang { partial class Object : Android.Runtime.IJavaObject {} } to "retrofit" `Java.Lang.Object` and `Java.Lang.Throwable` to use `IJavaObjectValueMarshaler`. There are three problems with this case: 1. It's not actually covered by unit tests! Removing the above `foreach` loop doesn't break any unit tests. 2. While dotnet/android was updated for this scenario, it was only as part of trying to use `jnimarshalmethod-gen` on Xamarin.Android projects, which was never finished. 3. It doesn't "scale": the code throws a `NotSupportedException` if a type implements more than one interface that has `[JniValueMarshaler]`. This isn't used, and *shouldn't* be relied upon. Excise it entirely. With these changes we can remove `DynamicallyAccessedMemberTypes.Interfaces`. I also introduced `build-tools/trim-analyzers/trim-analyzers.props` that will setup the appropriate trimmer MSBuild properties to make trimmer warnings an error. This should keep us from accidentally creating warnings. I only use this setting in projects that were already using `$(EnableAotAnalyzer)`.
6cc1754
to
6398552
Compare
The current implementation: return Java.Interop.TypeManager.CreateInstance (handle, transfer, type); Needs to become this, for NativeAOT support: JniObjectReference reference = JNIEnv.CreateJniObjectReference (handle, transfer); JniObjectReferenceOptions options = JNIEnv.ToJniObjectReferenceOptions (transfer); return (IJavaPeerable) JNIEnvInit.AndroidValueManager?.GetValue (ref reference, options, type); This way we use the appropriate `AndroidValueManager` for the runtime that is currently being used. If we just change the code in `Java.Lang.Object` to do this for all runtimes, this can keep everything a bit simpler. The fallout of this change requires `type` to be decorated with: [DynamicallyAccessedMembers (Constructors)] This trickled to create many other trimmer warnings, that are likely a real issue. This is a good opportunity to fix them. I also defined the `const` value in `Java.Lang.Object`: internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors; Which allowed many types that extend `Java.Lang.Object` able to reuse this `const` value and not define it again. I also had to make a helper function to map `JniHandleOwnership` -> `JniObjectReferenceOptions`, such as: internal static JniObjectReferenceOptions ToJniObjectReferenceOptions (JniHandleOwnership transfer) And `CreateJniObjectReference()`: internal static JniObjectReference CreateJniObjectReference (IntPtr handle, JniHandleOwnership transfer) Other changes: * Bump to dotnet/java-interop@2c06b3c2a Changes: dotnet/java-interop@f800ea5...2c06b3c
6398552
to
2fea602
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
return Java.Interop.TypeManager.CreateInstance (handle, transfer, type); | ||
JniObjectReference reference = JNIEnv.CreateJniObjectReference (handle, transfer); | ||
JniObjectReferenceOptions options = JNIEnv.ToJniObjectReferenceOptions (transfer); | ||
return (IJavaPeerable) JNIEnvInit.AndroidValueManager?.GetValue (ref reference, options, type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong about always wanting this cast.
That said, IIRC a previous version of this PR instead used as IJavaPeerable
, which also seems wrong.
The problem: Java.InteropTests.JnienvTest.MoarThreadingTests
is failing:
No exception should be thrown [t2]! Got: System.InvalidCastException: Arg_InvalidCastException
at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
at Android.Runtime.JNIEnv.<CreateNativeArrayElementToManaged>g__GetObject|74_11(IntPtr , Type )
at Android.Runtime.JNIEnv.<>c.<CreateNativeArrayElementToManaged>b__74_9(Type type, IntPtr source, Int32 index)
at Android.Runtime.JNIEnv.GetObjectArray(IntPtr , Type[] )
at Java.InteropTests.JnienvTest.<>c__DisplayClass26_0.<MoarThreadingTests>b__1()
Expected: null
But was: <System.InvalidCastException: Arg_InvalidCastException
at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
at Android.Runtime.JNIEnv.<CreateNativeArrayElementToManaged>g__GetObject|74_11(IntPtr , Type )
at Android.Runtime.JNIEnv.<>c.<CreateNativeArrayElementToManaged>b__74_9(Type type, IntPtr source, Int32 index)
at Android.Runtime.JNIEnv.GetObjectArray(IntPtr , Type[] )
at Java.InteropTests.JnienvTest.<>c__DisplayClass26_0.<MoarThreadingTests>b__1()>
android/tests/Mono.Android-Tests/Java.Interop/JnienvTest.cs
Lines 363 to 386 in 8d77130
IntPtr lrefJliArray = JNIEnv.NewObjectArray<int> (new[]{1}); | |
IntPtr grefJliArray = JNIEnv.NewGlobalRef (lrefJliArray); | |
JNIEnv.DeleteLocalRef (lrefJliArray); | |
Exception ignore_t1 = null; | |
Exception ignore_t2 = null; | |
var t1 = new Thread (() => { | |
int[] output_array1 = new int[1]; | |
for (int i = 0; i < 2000; ++i) { | |
Console.WriteLine ("# t1 iter: {0}", i); | |
try { | |
JNIEnv.CopyObjectArray (grefJliArray, output_array1); | |
} catch (Exception e) { | |
ignore_t1 = e; | |
break; | |
} | |
} | |
}); | |
var t2 = new Thread (() => { | |
for (int i = 0; i < 2000; ++i) { | |
Console.WriteLine ("# t2 iter: {0}", i); | |
try { | |
JNIEnv.GetObjectArray (grefJliArray, new[]{typeof (int)}); |
JnienvTest.cs:386
is the source of the JNIEnv.GetObjectArray()
invocation at the root of the stack trace.
The cast on this line is the source of the InvalidCastException
.
The question is: how did this previously work, and why is it failing now?
Tracing through how it previously worked:
JNIEnv.GetObjectArray()
is called;element_types=new[]{typeof(int)}
.JNIEnv.GetObjectArray()
callsGetConverter<T>(NativeArrayElementToManaged, null, array_ptr)
GetConverter<T>()
sees thatarray_ptr
/array
is not null; callsGetClassNameFromInstance(array)
.GetClassNameFromInstance(array)
should be[Ljava/lang/Object;
, because the array was created viaJNIEnv.NewObjectArray<int> (new[]{1})
.GetConverter<T>()
thus skips theswitch
on lines 732-749GetConvter<T>()
should thus return aFunc<Type?, IntPtr, int, object?>
forIJavaObject
:android/src/Mono.Android/Android.Runtime/JNIEnv.cs
Lines 703 to 708 in 8d77130
{ typeof (IJavaObject), (type, source, index) => { AssertIsJavaObject (type); IntPtr elem = GetObjectArrayElement (source, index); return Java.Lang.Object.GetObject (elem, JniHandleOwnership.TransferLocalRef, type); } }, - Returning to
GetObjectArray()
, line 1110targetType
will betypeof(int)
, we callconverter(null, array_ptr, i)
. value
should thus be ajava.lang.Integer
/Java.Lang.Integer
instance, and theConvert.ChangeType()
on line should turn it into anint
, asInteger
implementsIConvertible
.
So why's this change fail? Probably because AndroidValueManager?.GetValue(…)
has different semantics from TypeManager.CreateInstance(…)
. I need to further investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So why's this change fail? Probably because
AndroidValueManager?.GetValue(…)
has different semantics fromTypeManager.CreateInstance(…)
. I need to further investigate.
Time for further investigation! Consider the following patch to java-interop/samples/Hello-Java.Base:
diff --git a/samples/Hello-Java.Base/Program.cs b/samples/Hello-Java.Base/Program.cs
index b1e0d5ba..9e63f3ba 100644
--- a/samples/Hello-Java.Base/Program.cs
+++ b/samples/Hello-Java.Base/Program.cs
@@ -61,10 +61,15 @@ namespace Hello
CreateJLO ();
}
- static void CreateJLO ()
+ static unsafe void CreateJLO ()
{
- var jlo = new Java.Lang.Object ();
- Console.WriteLine ($"binding? {jlo.ToString ()}");
+ var i_class = new JniType ("java/lang/Integer");
+ var i_ctor = i_class.GetConstructor ("(I)V");
+ JniArgumentValue* i_args = stackalloc JniArgumentValue [1];
+ i_args [0] = new JniArgumentValue (42);
+ var i_value = i_class.NewObject (i_ctor, i_args);
+ var v = JniEnvironment.Runtime.ValueManager.GetValue (ref i_value, JniObjectReferenceOptions.CopyAndDispose, null);
+ Console.WriteLine ($"v? {v} {v?.GetType ()}");
}
static void ReportTiming ()
This creates a new java.lang.Integer(42)
, invokes Runtime.ValueManager.GetValue()
, and prints the resulting value and its type.
The output is:
% ~/Downloads/dotnet-sdk-8.0.206-osx-x64/dotnet run --project samples/Hello-Java.Base/*.csproj
…
v? 42 System.Int32
(I have to use .NET 8.0.206 because anything later crashes. Hopefully this will be fixed in the January .NET 8 service release…)
Note that Runtime.GetValue(objref, …, null)
converts to a System.Int32
, not a Java.Lang.Integer
. This is why there's an InvalidCastException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the cause of the InvalidCastException
is understood, what do we do about it?
There are at least two paths worth pursuing:
- Don't Do That™ (don't use
ValueManager.GetValue()
here) - Don't Do That™ (update
ValueManager.GetValue()
to not auto-unbox like that.)
(1) should be possible, by e.g. updating TypeManager.CreateInstance()
to use methods on JniEnvironment.Runtime.TypeManager
instead of using the P/Invokes within GetClassName()
/etc.
(2) should also be possible, but may be more time consuming, depending on how many unit tests it breaks.
Context: 8d77130 Context: #9630 We're exploring how to get .NET for Android apps to build and run using [Native AOT][0]. Default to `$(TrimMode)=Full` for NativeAOT, this enables trimmer warnings and should be the default mode for NativeAOT. Ensure the `_PrepareLinking` MSBuild target runs at the appropriate time. Failure to do so was causing `Android.App.Activity.GetOnCreate_Landroid_os_Bundle_Handler()` to be trimmed away. Various fixes to avoid `java.lang.UnsatisfiedLinkError` errors: * Set `$(LinkerFlavor)=lld` by default to avoid: E AndroidRuntime: java.lang.UnsatisfiedLinkError: dlopen failed: cannot locate symbol "__start___modules" referenced by "/data/app/~~_ggpMC4foLk_jUUycm0CfA==/net.dot.hellonativeaot-fvszIWroqgweLHYgULxVoQ==/split_config.arm64_v8a.apk!/lib/arm64-v8a/libNativeAOT.so"... * Include `libc++_shared.so` within the app, as Native AOT output requires C++: E AndroidRuntime: java.lang.UnsatisfiedLinkError: dlopen failed: library "libc++_shared.so" not found: needed by /data/app/~~_W0B9EE3hhajnFvCHyUKSg==/net.dot.hellonativeaot-zlXemqHdkbHaLu60oYPVQQ==/lib/arm64/libNativeAOT.so in namespace clns-6 Emit `JavaPeerStyle.JavaInterop1` java stubs for NativeAOT, as it is easier to get working in place of `JavaPeerStyle.XAJavaInterop1`. Specifically, we need to be able to avoid P/Invokes related to typemaps/etc. XAJavaInterop1 hits `JNIEnvInit.RegisterJniNatives()`, which is full of P/Invokes such as `TypeManager.GetClassName()`, while `JavaInterop1` hits `ManagedPeer.RegisterNativeMembers()` which goes through `JniRuntime` abstractions, allowing for a P/Invoke-free code path. I updated `BuildTest2.NativeAOT()` to assert for these changes where possible. [0]: https://learn.microsoft.com/dotnet/core/deploying/native-aot/
This does not appear to be needed for "Hello World" in #9636, so we'll revisit this later. It will likely be needed if we run |
The current implementation:
Needs to become this, for NativeAOT support:
This way we use the appropriate
AndroidValueManager
for the runtimethat is currently being used. If we just change the code in
Java.Lang.Object
to do this for all runtimes, this can keepeverything a bit simpler.
The fallout of this change requires
type
to be decorated with:This trickled to create many other trimmer warnings, that are likely a
real issue. This is a good opportunity to fix them.
I also defined the
const
value inJava.Lang.Object
:Which allowed many types that extend
Java.Lang.Object
able to reusethis
const
value and not define it again.I also had to make a helper function to map
JniHandleOwnership
->JniObjectReferenceOptions
, such as:And
CreateJniObjectReference()
:Other changes:
Changes: dotnet/java-interop@f800ea5...2c06b3c